-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Fix parallel rustc not being reproducible due to unstable sorts of items #144722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
I don't think the changes of diagnostic orders are expected.😅 |
b7e0813
to
a03edc6
Compare
This comment has been minimized.
This comment has been minimized.
MonoItem::Static(def_id) => def_id.as_local().map(Idx::index), | ||
MonoItem::GlobalAsm(item_id) => Some(item_id.owner_id.def_id.index()), | ||
}, | ||
local_item_query(item, |def_id| tcx.def_span(def_id)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try to use def_ident_span
or find_ancestor_not_from_extern_macro
to avoid having to shuffle tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice. I'll try it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def_ident_span
failed on tests/assembly-llvm/emit-intel-att-syntax.rs
about naked and global asm.
find_ancestor_not_from_macro
works here.
a03edc6
to
d6294cc
Compare
This comment has been minimized.
This comment has been minimized.
If we don't sort items again, it would broke existing tests. How about Edit: querying span makes a significant regression for perf. |
a9ba3d9
to
989e39f
Compare
This comment has been minimized.
This comment has been minimized.
989e39f
to
9e4f296
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Fix parallel rustc not being reproducible due to unstable sorts of items
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (77b6bc0): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.4%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.4%, secondary 3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 467.149s -> 473.005s (1.25%) |
This comment has been minimized.
This comment has been minimized.
Fix parallel rustc not being reproducible due to unstable sorts of items try-job: apple
💔 Test for 4507c18 failed: CI. Failed job:
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
@bors2 try jobs=build-aarch64-apple |
This comment has been minimized.
This comment has been minimized.
Fix parallel rustc not being reproducible due to unstable sorts of items try-job: build-aarch64-apple
💔 Test for 7339c51 failed: CI. Failed job:
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
@bors2 try jobs=dist-aarch64-apple |
Fix parallel rustc not being reproducible due to unstable sorts of items try-job: dist-aarch64-apple
This comment has been minimized.
This comment has been minimized.
@bors2 try cancel |
Try build cancelled. Cancelled workflows: |
@bors2 try jobs=aarch64-apple |
Fix parallel rustc not being reproducible due to unstable sorts of items try-job: aarch64-apple
This comment has been minimized.
This comment has been minimized.
@bors r+ |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 1c9952f (parent) -> 350d0ef (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 350d0ef0ec0493e6d21cfb265cb8211a0e74d766 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (350d0ef): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.0%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.5%, secondary 3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 465.78s -> 468.291s (0.54%) |
Currently, A tuple
(DefId, SymbolName)
is used to determine the order of items in the final binary. HoweverDefId
is expected as non-deterministic, which leads to some not reproducible issues under parallel compilation. (See #140425 (comment))Theoretically, we don't need the sorting because the order of these items is already deterministic.
However, codegen tests reply on the same order of items between in binary and source.
So here we added a new option
codegen-source-order
to indicate whether sorting based on the order in source. For codegen tests, items are sorted according to the order in the source code, whereas in the normal path, no sorting is performed.Specially, for codegen tests, in preparation for parallel compilation potentially being enabled by default in the future, we use
Span
replacingDefId
to make the order deterministic.This PR is purposed to fix #140425, but seemly works on #140413 too.
This behavior hasn't added into any test until we have a test suit for the parallel frontend. (See #143953)
Related discussion: Zulip #144576
Update #113349
r? @oli-obk
cc @lqd @cramertj @matthiaskrgr @Zoxc @SparrowLii @bjorn3 @cjgillot @joshtriplett